Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-6535 DynamoRevit Improve Search #14878

Merged
merged 5 commits into from
Jan 25, 2024
Merged

DYN-6535 DynamoRevit Improve Search #14878

merged 5 commits into from
Jan 25, 2024

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

Improving Dynamo search on Revit
In a search criteria like "list replace" I've implemented a fix that will check how many nodes are shown when using the terms splitted by empty space (in this case list and replace) so if it reaches the limit then we discard the split search and execute a normal search (without empty spaces).

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Improving Dynamo search on Revit

Reviewers

@QilongTang @reddyashish

FYIs

In a search criteria like "list replace" I've implemented a fix that will check on many nodes we have when the terms splitted by empty space (in this case list and replace)  so if it reaches the limit then we discard the split search and execute a normal search.
@RobertGlobant20
Copy link
Contributor Author

This is a Screenshot of the search results before my fix.
image

@RobertGlobant20
Copy link
Contributor Author

This is a screenshot of search results after my fix.
image

else
searchType = SearchType.Normal;

var trimmedSearchTerm = searchType == SearchType.ByEmptySpace ? searchTerm.Replace(" ", "") : searchTerm;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the actual node name has spaces in it? Will those show up with this condition?
Also since the ByEmptySpace is just used to check if the search term has to be trimmed or not, I don't think it should be a value inside SearchType enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reddyashish for the first question I think the Tokenizer that we are using is removing the empty spaces, just to confirm I've tested in DynamoSandbox with several nodes that use empty spaces and I'm getting the expected node always in first place (see image below) so there should not be any problem with nodes having empty space.
image

For the second comment we are using SearchType.ByEmptySpace in more conditions in the code that's why I considered to add it in the SearchType enum.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the confirmation @RobertGlobant20. Can't we just use that as a property to check for empty spaces then? Because the search algorithm is not altered, just the search term. Might not be a big thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converted to be a property in the next commit: ff1b100

Convert the ByEmptySpace to be a property and remove it from the enum.
Copy link

github-actions bot commented Jan 23, 2024

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@QilongTang QilongTang added this to the 3.1 milestone Jan 24, 2024
@@ -82,6 +84,15 @@ public enum LuceneStorage
FILE_SYSTEM
}

public enum SearchType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments for public fields please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added extra comments: 584f794

@reddyashish reddyashish merged commit f33a015 into DynamoDS:master Jan 25, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants